This repository was archived by the owner on Jun 3, 2026. It is now read-only.
fix: warn on legacy Bedrock cache options#1055
Open
asim48-ctrl wants to merge 1 commit into
Open
Conversation
asim48-ctrl
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
asim48-ctrl
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
github-actions
Bot
added
the
strands-running
Contributor
|
Assessment: Approve Clean, minimal fix that addresses the issue well. The warning message follows the project's logging style guide, Minor suggestion
|
github-actions
Bot
removed
the
strands-running
poshinchen
approved these changes
May 22, 2026
notowen333
suggested changes
May 22, 2026
| if (legacyCacheOptions?.cachePrompt !== undefined || legacyCacheOptions?.cacheTools !== undefined) { | ||
| warnOnce( | ||
| logger, | ||
| 'unsupported_fields=<cachePrompt,cacheTools> | cachePrompt and cacheTools are not supported by BedrockModel, use cacheConfig instead' |
Contributor
There was a problem hiding this comment.
1. (Functional gap) updateConfig is not covered. The same legacy fields can
land via updateConfig({ cachePrompt: 'default' } as any) (bedrock.ts:513) —
that path silently drops them just like the constructor used to. If the goal
is "users shouldn't believe caching is enabled when it isn't," updateConfig
should warn too. Minor refactor: extract a warnIfLegacyCacheOptions(options)
helper and call it in both places.
2. (Test placement) The new test lives in the wrong describe block. Per the
diff context (@@ -448,6 +448,18 @@), the it('warns when unsupported legacy
cache options...') is inserted at the tail of describe('getConfig')
(bedrock.test.ts:437), not describe('constructor') (line 183), where the
analogous it('warns when modelId is not explicitly set') test already lives
(line 190). Move it next to the other constructor-warning tests for
discoverability.
3. (Message accuracy) Warning text claims both fields when only one may be
present. If a caller passes only cachePrompt, the warning still says
unsupported_fields=<cachePrompt,cacheTools>. Either:
- List only the keys actually present (more accurate, costs warnOnce dedup
since the key set varies), or
- Keep the static message but drop the redundancy (the structured prefix and
prose say the same thing twice).
Collaborator
|
This repository has been merged into the strands-agents/harness-sdk monorepo and will be archived shortly. All new development happens there. If this PR is still relevant, please recreate it against the monorepo. The code now lives under Apologies for the disruption, and thank you for contributing! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This adds a small runtime warning when
BedrockModelreceives the stalecachePromptorcacheToolsfields documented on the older indexed API page. Those fields are not honored by the current provider, so the warning points users tocacheConfiginstead of silently leaving Bedrock prompt caching disabled.Fixes #1027.
Testing